Issue 56128 segment id ice nightly #56143

nikomatsakis · 2018-11-21T19:23:30Z

Tentative fix for #56128

From what I can tell, the problem is that if you have pub(super) use foo::{a, b}, then when we explode the a and b, the segment ids from the super path were not getting cloned. However, once I fixed that, then I ran into a problem that the "visibility" node-ids were not present in the final HIR -- this is because the visibility of the "stem" that is returned in this case was getting reset to inherited. I don't think it is a problem to undo that, so that the visibility is returned unmodified.

Fixes #55475
Fixes #56128

cc @nrc @petrochenkov

rust-highfive · 2018-11-21T19:23:33Z

r? @varkor

(rust_highfive has picked a reviewer for you, use r? to override)

rust-highfive · 2018-11-21T19:23:34Z

⚠️ Warning ⚠️

These commits modify submodules.

nikomatsakis · 2018-11-21T19:23:46Z

r? @petrochenkov

nrc

Did you mean to update the submodules?

LGTM otherwise

nikomatsakis · 2018-11-21T20:33:20Z

Er, no, no I did not.

Without this, the `vis` does not wind up in the tree anywhere, and then we get ICEs because the node-ids it refers to are not present. The motivation seemed to be documentation, but `ListStem` HIR nodes are ignored in rustdoc, from what I can tell.

We are not mutating it now.

nikomatsakis · 2018-11-21T20:35:03Z

I guess though that this is fallout from the visibility change:

[00:48:29] ---- [ui] ui/lint/unreachable_pub-pub_crate.rs stdout ----
[00:48:29] diff of stderr:
[00:48:29] 
[00:48:29] 14    = help: or consider exporting it for use by other crates
[00:48:29] 15 
[00:48:29] 16 warning: unreachable `pub` item
[00:48:29] +   --> $DIR/unreachable_pub-pub_crate.rs:26:5
[00:48:29] +    |
[00:48:29] + LL |     pub use std::env::{Args}; // braced-use has different item spans than unbraced
[00:48:29] +    |     ---^^^^^^^^^^^^^^^
[00:48:29] +    |     |
[00:48:29] +    |     help: consider restricting its visibility: `pub(crate)`
[00:48:29] +    |
[00:48:29] +    = help: or consider exporting it for use by other crates
[00:48:29] + 
[00:48:29] + warning: unreachable `pub` item
[00:48:29] 18    |
[00:48:29] 18    |
[00:48:29] 19 LL |     pub use std::env::{Args}; // braced-use has different item spans than unbraced

nikomatsakis · 2018-11-21T20:35:16Z

I can make that code ignore ListStem but..ugh.

nikomatsakis · 2018-11-21T21:10:21Z

I tried an alternate fix (use the vis for the last item) but it ice'd because the owner was wrong; more notes from discord here.

I don't have a better idea than what's in this PR -- I leave it to @nrc and @petrochenkov to decide best course of action =)

alexcrichton · 2018-11-21T22:09:01Z

@bors: p=1

we're gonna want to backport this

petrochenkov · 2018-11-22T00:06:41Z

src/librustc/hir/lowering.rs

                let def = self.expect_full_def_from_use(id).next().unwrap_or(Def::Err);
                let path = P(self.lower_path_extra(def, &prefix, ParamMode::Explicit, None));
-                *vis = respan(prefix.span.shrink_to_lo(), hir::VisibilityKind::Inherited);


IIRC, you still need to reset at least pub (hir::VisibilityKind::Public) into something more private, otherwise you'll get a lot of noise from rustdoc (the stem is emitted for all braced imports).

This way the hack for UnreachablePub can be avoided as well.

This should be ok since hir::VisibilityKind::Public doesn't have a path with node IDs inside it.

Everything except for VisibilityKind::Restricted can be reset to VisibilityKind::Inherited basically.

petrochenkov · 2018-11-22T00:12:34Z

Beside #56143 (comment) everything looks reasonable.

(I never fully understood what are relations between node ids, def ids, their owners, parents, etc though, and why @QuietMisdreavus had to do all those horrible things with them in #51425.)

petrochenkov · 2018-11-22T20:14:05Z

@bors r+

bors · 2018-11-22T20:14:06Z

📌 Commit 5f2a173 has been approved by petrochenkov

bors · 2018-11-22T20:29:58Z

⌛ Testing commit 5f2a173 with merge 00e03ee...

@nrc

…y, r=petrochenkov Issue 56128 segment id ice nightly Tentative fix for #56128 From what I can tell, the problem is that if you have `pub(super) use foo::{a, b}`, then when we explode the `a` and `b`, the segment ids from the `super` path were not getting cloned. However, once I fixed *that*, then I ran into a problem that the "visibility" node-ids were not present in the final HIR -- this is because the visibility of the "stem" that is returned in this case was getting reset to inherited. I don't *think* it is a problem to undo that, so that the visibility is returned unmodified. Fixes #55475 Fixes #56128 cc @nrc @petrochenkov

bors · 2018-11-22T23:25:22Z

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing 00e03ee to master...

@ghost

[beta] Rollup backports * #56163: [master] Backport 1.30.1 release notes * #56147: resolve: Fix some asserts in import validation * #56118: Update books for Rust 2018 * #56117: resolve: Make "empty import canaries" invisible from other crates * #56065: Replace the ICEing on const fn loops with an error * #56143: Issue 56128 segment id ice nightly * #56134: Fix clippy documentation links (first in #56156) r? @ghost

nikomatsakis added 3 commits November 21, 2018 13:41

track the span for each id so that we can give a nice ICE

d0a174d

add some debug! into lowering

40f8094

renumber segment ids for visibilities whenever we clone them

a0a4790

rust-highfive assigned varkor Nov 21, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 21, 2018

rust-highfive assigned petrochenkov and unassigned varkor Nov 21, 2018

This was referenced Nov 21, 2018

Panic rustc 1.31.0-nightly (96064eb61 2018-10-28) running on x86_64-pc-windows-msvc: in hir_id_validator.rs:31 #55475

Closed

Beta ICE: ItemLocalIds not assigned densely #56128

Closed

alexcrichton added this to the Rust 2018 Release milestone Nov 21, 2018

alexcrichton added beta-nominated Nominated for backporting to the compiler in the beta channel. beta-accepted Accepted for backporting to the compiler in the beta channel. labels Nov 21, 2018

This comment has been minimized.

Sign in to view

nrc reviewed Nov 21, 2018

View reviewed changes

nikomatsakis added 3 commits November 21, 2018 15:34

add regression test

2bd2fc9

pass vis by shared reference

4c7ce7c

We are not mutating it now.

nikomatsakis force-pushed the issue-56128-segment-id-ice-nightly branch from 3440713 to 4c7ce7c Compare November 21, 2018 20:34

hack: ignore list-stems for pub lint

9cdf491

This comment has been minimized.

Sign in to view

add compile-pass annotation

ebf3c8d

petrochenkov reviewed Nov 22, 2018

View reviewed changes

nikomatsakis added 2 commits November 22, 2018 09:38

only reset non-restricted visibilities

b83150e

explain how this works

5f2a173

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 22, 2018

pietroalbini mentioned this pull request Nov 22, 2018

[beta] Rollup backports #56174

Merged

bors merged commit 5f2a173 into rust-lang:master Nov 22, 2018

alexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Nov 23, 2018

DJMcNab mentioned this pull request Dec 9, 2018

ra_syntax fails to compile with rustc 1.31.0-nightly (96064eb 2018-10-28) rust-lang/rust-analyzer#185

Closed

ljedrz mentioned this pull request Feb 14, 2019

HirId-ify intravisit #58232

Merged

Issue 56128 segment id ice nightly #56143

Issue 56128 segment id ice nightly #56143

Uh oh!

Conversation

nikomatsakis commented Nov 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Nov 21, 2018

Uh oh!

rust-highfive commented Nov 21, 2018

Uh oh!

nikomatsakis commented Nov 21, 2018

Uh oh!

This comment has been minimized.

nrc left a comment

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented Nov 21, 2018

Uh oh!

nikomatsakis commented Nov 21, 2018

Uh oh!

nikomatsakis commented Nov 21, 2018

Uh oh!

nikomatsakis commented Nov 21, 2018

Uh oh!

alexcrichton commented Nov 21, 2018

Uh oh!

This comment has been minimized.

petrochenkov Nov 22, 2018

Choose a reason for hiding this comment

Uh oh!

petrochenkov Nov 22, 2018

Choose a reason for hiding this comment

Uh oh!

petrochenkov commented Nov 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petrochenkov commented Nov 22, 2018

Uh oh!

bors commented Nov 22, 2018

Uh oh!

bors commented Nov 22, 2018

Uh oh!

bors commented Nov 22, 2018

Uh oh!

Uh oh!

nikomatsakis commented Nov 21, 2018 •

edited

Loading

petrochenkov commented Nov 22, 2018 •

edited

Loading